[SSF-145] - Cognito integration account creation#113
[SSF-145] - Cognito integration account creation#113dburkhart07 wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Great first pass!
In addition to the comments I left, I found one more bug I wanted to mention. After I create an account as a pantry, the first time I try to access the dashboard after creating my account I get the following problem:
It's fine after I refresh though
| const createUserDto: userSchemaDto = { | ||
| email: pantry.pantryUser.email, | ||
| firstName: pantry.pantryUser.firstName, | ||
| lastName: pantry.pantryUser.lastName, | ||
| phone: pantry.pantryUser.phone, | ||
| role: Role.PANTRY, | ||
| }; |
There was a problem hiding this comment.
this dto is being created multiple times, think we can remove some repeat code
There was a problem hiding this comment.
not quite sure i understand this. are you referencing that we create this dto in the pantries and fm service, and because of that we should abstract it? if thats the case, im not quite sure if that abstraction is really going to save much repeat code. perhaps i am missing something though.
| firstName, | ||
| lastName, | ||
| email, | ||
| phone, |
There was a problem hiding this comment.
if phone is here maybe it should be included in the dto? but then we should send it to cognito.
i guess the question is what's the purpose of including the phone number:
- cognito uses it for mfa or invitation messages
- db stores it for some other reason
There was a problem hiding this comment.
i personally am fine with removing it. if we plan on adding an option for mfa verification down the line, maybe its good to include this. if not (or we just want to use email verification), then i think we are fine to just remove it in this case. @sam-schu @Yurika-Kan, thoughts?
| return sub; | ||
| } catch (error) { | ||
| if (error.name == 'UsernameExistsException') { | ||
| throw new ConflictException('A user with this email already exists'); |
There was a problem hiding this comment.
if there is an email conflict doesn't it make more sense to notify the user when they submit the application rather than the admin who is approving/denying the application? there isn't really anything the admin can do if they find that an email already exists in the db other than reach out directly
There was a problem hiding this comment.
hmm this is a good question, and likely related to product.
for the pantry and food manufacturer flow: a DB user should be created with an empty cognito sub when the application is submitted. we could make an api call to check if the primary email exists in cognito and throw an error otherwise (since having a cognito account makes them an official user) and not allow for modal submission.
for admin and volunteers: this is a little harder, since i am not quite sure what our frontend flow is going to be for creating these two yet. how is this email going to be obtained from the frontend?
| { Name: 'email', Value: email }, | ||
| { Name: 'email_verified', Value: 'true' }, | ||
| ], | ||
| DesiredDeliveryMediums: ['EMAIL'], |
There was a problem hiding this comment.
I think we should change the email from the default to 1) indicate that their application was approved and 2) provide further information other than simply the password, like how to access the signup page, or even a link to the application. When I got the email I wasn't quite sure what to do with it
You can look at my cognito docs for instructions on how to change the default email, lmk if you have any questions
There was a problem hiding this comment.
how to access this page will definitely change, but we can just put the link as localhost for right now, and change it later, so i like the idea of more information on what to do with the password as well.
the application approved sounds good too, but im not sure if there is a separate automated email for that. i know in many modern flows (for example, email verification after you submit a job application), you receive 2 emails: one saying your application was submitted, and another being to verify your email. if we have a separate one for this, i suggest we keep the template basic on how to setup the account.
2 participants
ℹ️ Issue
Closes #145
📝 Description
This PR goes into the process of creating a new user through Cognito. Our existing Auth Service that utilized the Cognito API required a password, so we needed to use a new one that generates a temporary password, and emails it to the new user that we want to create.
When the user enters their temporary password for the first time, amplify stores a challenge that tells us that we need to create a new permanent password. Because of this, we needed to add in a new frontend modal into what we already had. From there, we can get the user cognito client sub, which is what we now store in our database.
The pantry and food manufacturer flow is slightly different, as they have linked users which are already created as they wait for approval after filling out the application. Because of that, we implement a check to see what their role is to determine if we update it in the database, or create a new user.
✔️ Verification
Run the following steps:
🏕️ (Optional) Future Work / Notes
Pending testing being written for this.